Add websocket modes to dotnet-dsrouter #3461

lambdageek · 2022-10-13T17:55:23Z

In .NET 8, the .NET runtime may ship a variant of the WebAssembly runtime that supports diagnostic tracing (this is an optional opt-in because it requires threading support, which is opt-in). Runtime tracking issue: dotnet/runtime#69268

In order to support tracing in WebAssembly apps, the dotnet-dsrouter tool needs a new mode where it can create a WebSocket server that will allow a .NET app running in the browser to connect to it.

$ dotnet-dsrouter server-websocket -ipcs ./domain_socket -ws ws://localhost:12345/diagnostics

After that, the usual diagnostics commands such as dotnet-trace will be able to collect diagnostics from running WebAssembly apps.

$ dotnet-trace collect --diagnostic-port ./domain_socket,connect ...

All the remaining work is done or pushed off to the next PR

done ~~cleanup rogue Console.WriteLines from the initial development~~
done ~~thread ILoggerFactory through from dotnet-dsrouter down to WebSocketServer (through the diagnostic client library) to allow Kestrel to log at the same levels as the rest of the tool.~~ pass logging level through to the generic host builder
out of scope Spinning out the websocket handling into a DI middleware - in some scenarios we actually want to load it into an existing server (browsers don't always allow you to connect to arbitrary websocket url's if oyu're not deployed on localhost) Will make the library suitable for the wasm dev server in a future PR
done ~~Find a better way to instantiate the WebSocketServer than by setting an environment variable and using Activator.CreateInstance~~ the client library already has IVT for dotnet-dsrouter, so we can just use an internal singleton to set a webserver factory.

Questions:

Should we consider targeting netcoreapp3.1 instead of net6? The issue that all the DI stuff for setting up the generic host and starting Kestrel is different for .NET Core 3.1 and .NET 6. Only dotnet-dsrouter is changing, all the other libraries are still 3.1.

Fixes #3422

src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcWebSocketServerTransport.cs

lambdageek · 2022-10-13T22:42:04Z

@lateralusX Could you take a look. Not a detailed review yet, but maybe your high-level impression.

lambdageek

I copy/pasted a bunch of code, based on what the Tcp and Icp transports do. But this could probably do with some abstract base classes that provide shared behavior.

Somehow the abstractions aren't a good fit for where the code actuall differs.

...crosoft.Diagnostics.NETCore.Client/DiagnosticsServerRouter/DiagnosticsServerRouterFactory.cs

src/Tools/dotnet-dsrouter/DiagnosticsServerRouterCommands.cs

src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcWebSocketEndPoint.cs

...crosoft.Diagnostics.NETCore.Client/DiagnosticsServerRouter/DiagnosticsServerRouterFactory.cs

src/Microsoft.Diagnostics.WebSocketServer/WebSocketServer.cs

src/Tools/dotnet-dsrouter/Program.cs

lambdageek · 2022-10-20T13:04:12Z

@lateralusX @dotnet/dotnet-diag I think this ready for a review now

add an interface for it in the Client library, and expose a way to check that it's conncted.

Less copy/paste, more sharing

Use IpcServerTcpServerRouterFactory since everything is sufficiently abstracted

In this mode the tracing command will start a domain socket and the runtime will connect to it: ``` dotnet trace collect --diagnostic-port /tmp/sock ``` Then ``` dotnet run --project src/Tools/dotnet-dsrouter -- client-websocket -ipcc /tmp/sock -ws http://localhost:8088/diagnostics ``` And finally launch the app in the browser http://localhost:8000 (or whatever you configured) In this mode, the app should be configured without suspending on startup ``` <WasmExtraConfig Include="environmentVariables" Value=' { "DOTNET_DiagnosticPorts": "ws://localhost:8088/diagnostics", }' /> ```

it was just encapsulating a single URL parser call.

rename the class that encapsulated IHost and the embedded Kestrel instance to EmbeddedWebServer add comments explaining the various pieces

and IVT for Microsoft.Diagnostics.WebSocketServer that holds the implementation

lateralusX

Looks great! Nice work on the refactoring and code reuse aspects as well. Mainly some whitespace nits, and a couple of questions/comments coming to mind when reading through the code.

There are plans to upgrade dotnet tools/components to .net6 soon, but so far all are still on 3.1, made comments that these projects should probably follow the rest of the tools and components and build using 3.1 and upgraded once all tools/components move up to .net6, unless that is a major issue.

Small reflection on the name NetServerRouterFactory, maybe an alternative could have been SocketServerRouterFactory and then we could have TcpSocketServer, WebSocketServer, TcpSocketClient through the code, but that is just naming, if you don't agree we could stick with NetServerRouterFactory and/or change it at some later point in time.

src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcWebSocketServerTransport.cs

...crosoft.Diagnostics.NETCore.Client/DiagnosticsServerRouter/DiagnosticsServerRouterFactory.cs

src/Microsoft.Diagnostics.NETCore.Client/ReversedServer/ReversedDiagnosticsServer.cs

src/Microsoft.Diagnostics.WebSocketServer/EmbeddedWebSocketServer.cs

lateralusX · 2022-10-28T15:34:29Z

src/Microsoft.Diagnostics.WebSocketServer/Microsoft.Diagnostics.WebSocketServer.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFramework>net6.0</TargetFramework>


I guess this needs to be insync with rest of diagnostic repro, the plan was to bump, but not sure that has happened yet over the board.

src/Tools/dotnet-dsrouter/Program.cs

lateralusX · 2022-10-28T15:49:51Z

Btw, did you take changed dsrouter on a test ride against the regular tcp backend as well?

lambdageek · 2022-10-28T16:46:02Z

Btw, did you take changed dsrouter on a test ride against the regular tcp backend as well?

no. I'll be sure to try it after I fix the webserver lifetime.

Thanks for the review!

...crosoft.Diagnostics.NETCore.Client/DiagnosticsServerRouter/DiagnosticsServerRouterFactory.cs

Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>

Instead of trying to start the webserver when the diagnostics code first calls AcceptConnection, just start the webserver when we start running, and stop it when the router is done. This also matches the expected use by WasmAppHost (`dotnet run`) which will start/stop the webserver itself, and we will just provide middleware for diagnostics

src/Tools/dotnet-dsrouter/DiagnosticsServerRouterCommands.cs

lateralusX · 2022-11-14T11:49:10Z

@lambdageek Where you able to verify that things still works using the TCP backend after the change? If so, I believe this PR is good to go.

lambdageek · 2022-11-14T16:12:55Z

ok, confirmed dotnet-dsrouter server-server from this PR still works with Android. I did not break tcp server mode 😁

lateralusX

LGTM!

lateralusX · 2022-11-14T18:48:04Z

@tommcdon We need a sign off from dotnet/dotnet-diag before this PR can be merged.

hoyosjs

RE: .NET 6 - the only thing will be that it differs a bit from the expectation that the other tools have, so a bit rougher UX. Otherwise, I can't see anything where this would matter release-wise. We need to move to 6 anyway for the rest of the tools.

Mostly approving so you can merge - I didn't read too closely as I'm not really familiar with all the semantics for this.

src/Tools/dotnet-dsrouter/dotnet-dsrouter.csproj

lambdageek · 2022-11-15T20:53:04Z

@hoyosjs @lateralusX I'm not authorized to merge in this repo, could one of you do it

lambdageek commented Oct 13, 2022

View reviewed changes

src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcWebSocketServerTransport.cs Outdated Show resolved Hide resolved

lambdageek commented Oct 13, 2022

View reviewed changes

lambdageek commented Oct 17, 2022

View reviewed changes

src/Microsoft.Diagnostics.WebSocketServer/WebSocketServer.cs Outdated Show resolved Hide resolved

lateralusX reviewed Oct 18, 2022

View reviewed changes

src/Tools/dotnet-dsrouter/Program.cs Show resolved Hide resolved

lambdageek force-pushed the wasm-server branch from 377d90a to 9185d5b Compare October 19, 2022 18:57

lambdageek changed the title ~~[DRAFT] add server-websocket mode to dotnet-dsrouter~~ [DRAFT] add websocket modes to dotnet-dsrouter Oct 19, 2022

lambdageek marked this pull request as ready for review October 20, 2022 13:03

lambdageek requested a review from a team as a code owner October 20, 2022 13:03

lambdageek changed the title ~~[DRAFT] add websocket modes to dotnet-dsrouter~~ Add websocket modes to dotnet-dsrouter Oct 20, 2022

lambdageek and others added 19 commits October 26, 2022 13:31

Add "dsrouter server-websocket" command boilerplate

d147761

WIP: server-websocket mode, now with web server

cb2d059

debug WriteLines

a8f90c7

More WebSocketStreamAdapter work

f692c37

add an interface for it in the Client library, and expose a way to check that it's conncted.

fix comment

b5aabb8

One more IWebSocketStreamAdapter dynamic cast

7689a22

remove debug WriteLines

19e62c4

update after rebase

c4971df

Move WebSocketServer bits to a separate assembly outside dotnet-dsrouter

6f9897b

fix up names

873adf0

use a factory for creating WebSocketServer instances, instead of env var

50494ac

base class for TcpServerRouterFactory and WebSocketServerRouterFactory

ac3d707

Less copy/paste, more sharing

Remove IpcServerWebSocketServerRouterFactory

d42f90f

Use IpcServerTcpServerRouterFactory since everything is sufficiently abstracted

Remove more copy/pasted code

b492e6d

Suppress generic host startup messages

a2bffe8

Factor out common run loop for dsrouter commands

56d29e4

Remove development WriteLines

912f9ea

Remove IpcWebSocketEndPointer class

8b4b856

it was just encapsulating a single URL parser call.

lambdageek added 6 commits October 26, 2022 13:31

Pass LogLevel from dotnet-dsrouter to the webserver

9260d0c

Add comments, remove unused interface

836fe65

code organization cleanup

e0d928c

rename the class that encapsulated IHost and the embedded Kestrel instance to EmbeddedWebServer add comments explaining the various pieces

more comments

45fd0aa

fix line damage

11ad7b4

make the IWebSocketServer interface internal

5dca4f2

and IVT for Microsoft.Diagnostics.WebSocketServer that holds the implementation

lambdageek force-pushed the wasm-server branch from 4b2398c to 5dca4f2 Compare October 26, 2022 17:37

lateralusX reviewed Oct 28, 2022

View reviewed changes

lambdageek commented Oct 28, 2022

View reviewed changes

...crosoft.Diagnostics.NETCore.Client/DiagnosticsServerRouter/DiagnosticsServerRouterFactory.cs Outdated Show resolved Hide resolved

lambdageek and others added 3 commits October 28, 2022 12:54

Fix formatting and whitespace

d18b37e

Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>

Remove unnecessary logging

5124c7f

lateralusX reviewed Nov 1, 2022

View reviewed changes

src/Tools/dotnet-dsrouter/DiagnosticsServerRouterCommands.cs Show resolved Hide resolved

lateralusX self-requested a review November 14, 2022 16:46

lateralusX approved these changes Nov 14, 2022

View reviewed changes

hoyosjs approved these changes Nov 14, 2022

View reviewed changes

src/Tools/dotnet-dsrouter/dotnet-dsrouter.csproj Outdated Show resolved Hide resolved

lambdageek added 2 commits November 15, 2022 10:00

Use net6.0 logging for dotnet-dsrouter

5b36507

fix help string for dsrouter client-websocket command

f851bb3

hoyosjs merged commit dd56ba1 into dotnet:main Nov 15, 2022

github-actions bot locked and limited conversation to collaborators Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add websocket modes to dotnet-dsrouter #3461

Add websocket modes to dotnet-dsrouter #3461

lambdageek commented Oct 13, 2022 •

edited

Loading

lambdageek commented Oct 13, 2022

lambdageek left a comment

lambdageek commented Oct 20, 2022

lateralusX left a comment •

edited

Loading

lateralusX Oct 28, 2022

lateralusX commented Oct 28, 2022

lambdageek commented Oct 28, 2022

lateralusX commented Nov 14, 2022 •

edited

Loading

lambdageek commented Nov 14, 2022

lateralusX left a comment

lateralusX commented Nov 14, 2022

hoyosjs left a comment •

edited

Loading

lambdageek commented Nov 15, 2022

Add websocket modes to dotnet-dsrouter #3461

Add websocket modes to dotnet-dsrouter #3461

Conversation

lambdageek commented Oct 13, 2022 • edited Loading

lambdageek commented Oct 13, 2022

lambdageek left a comment

Choose a reason for hiding this comment

lambdageek commented Oct 20, 2022

lateralusX left a comment • edited Loading

Choose a reason for hiding this comment

lateralusX Oct 28, 2022

Choose a reason for hiding this comment

lateralusX commented Oct 28, 2022

lambdageek commented Oct 28, 2022

lateralusX commented Nov 14, 2022 • edited Loading

lambdageek commented Nov 14, 2022

lateralusX left a comment

Choose a reason for hiding this comment

lateralusX commented Nov 14, 2022

hoyosjs left a comment • edited Loading

Choose a reason for hiding this comment

lambdageek commented Nov 15, 2022

lambdageek commented Oct 13, 2022 •

edited

Loading

lateralusX left a comment •

edited

Loading

lateralusX commented Nov 14, 2022 •

edited

Loading

hoyosjs left a comment •

edited

Loading